Start building a universal System.Linq.Expressions library #61952
Conversation
System.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library: * `FEATURE_COMPILE` (defined everywhere except iOS/tvOS/Catalyst, and NativeAOT) - enables Reflection.Emit-based expression tree compiler. * `FEATURE_INTERPRET` (always defined and not actually possible to build without) - enables the expression interpreter * `FEATURE_DLG_INVOKE` (defined everywhere except NativeAOT, but we likely need to be able to run without it on iOS too because there's uninvestigated bugs around it ActiveIssue'd in dotnet#54970) - in the interpreter, use a delegate to invoke `CallInstructions` instead of `MethodInfo.Invoke`. The delegate might have to be Reflection.Emitted if it's not supportable with `Func`/`Action` (that's the uninvestigated iOS/tvOS/Catalyst bug). For dotnet#61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations _at publish time_ since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the `#define`s into feature switches. The feature switch is placed in the last proposed location for dotnet#17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again. This pull request is focused on the mechanical replacement of `#defines` with feature switches and it's already a lot bigger than I'm comfortable with. There's some obvious "`!FEATURE_COMPILE` means this is .NET Native with everything what that meant" that I did not touch because this is meant to be a mechanical change. Some cleanup will be needed at some point. Right now this just mostly means we're running fewer tests than we could. Validation: * Verified that we're still running the same number of tests with CoreCLR as we previously were and they're all passing. * Verified we're getting mostly the same size of the S.L.Expressions library on iOS (433 kB grew to 437 kB, the diffs are expected). * Verified things work on the NativeAOT side as well.
|
Tagging subscribers to this area: @cston Issue DetailsSystem.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library:
For #61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations at publish time since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the The feature switch is placed in the last proposed location for #17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again. This pull request is focused on the mechanical replacement of There's some obvious " Validation:
Cc @jkotas @marek-safar @MaximLipnin @steveisok @akoeplinger
|
src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/DelegateHelpers.cs
Outdated
Show resolved
Hide resolved
|
/azp run runtime-manual |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| <type fullname="System.Dynamic.Utils.DelegateHelpers"> | ||
| <method signature="System.Boolean get_CanEmitObjectArrayDelegate()" feature="System.Linq.Expressions.CanEmitObjectArrayDelegate" featurevalue="false" body="stub" value="false" /> | ||
| </type> | ||
| <type fullname="System.Linq.Expressions.Interpreter.CallInstruction"> | ||
| <method signature="System.Boolean get_CanCreateArbitraryDelegates()" feature="System.Linq.Expressions.CanCreateArbitraryDelegates" featurevalue="false" body="stub" value="false" /> | ||
| </type> |
There was a problem hiding this comment.
Shouldn't these be RuntimeFeature(s) instead?
There was a problem hiding this comment.
Property living in Expressions was the last direction #17973 (comment) discussed. I'm just following that, given the API is unreviewed.
get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates probably won't be APIs ever. CanCompileToIL might.
There was a problem hiding this comment.
I don't understand exactly what they try to do but it seems to me they react to runtime limitations, right? Should not this be then call to runtime to get the actual value with a feature switch on top of it?
There was a problem hiding this comment.
get_CanEmitObjectArrayDelegate and get_CanCreateArbitraryDelegates could probably be replaced with RuntimeFeature.IsDynamicCodeSupported eventually, but it would be a behavior change and this pull request didn't intend to do behavior changes, just replace #defines with something that can be overriden.
Replacing get_CanCreateArbitraryDelegates with IsDynamicCodeSupported would make the iDevices version take the codepaths that correspond to FEATURE_DLG_INVOKE not being defined in the csproj and that's not how iDevices build was set up in #54970. If you would like me to make that change, I can do it, but I cannot investigate any possible fallouts from it because I don't work on or own iDevices. An observable difference will be slower perf when invoking method call instructions in the interpreter. An upside will be that some of the tests that were blocked in #54970 will work. Ideally all of this (configuring the library so that it properly works, and revisiting tests blocked in #54970) should happen in subsequent pull requests. I definitely plan to revisit the blocked tests.
get_CanEmitObjectArrayDelegate cannot be replaced with RuntimeFeature.IsDynamicCodeSupported at this moment because it requires a runtime API that is not implemented on Mono and would therefore break iDevices (it's an API to create a strongly typed Delegate that boxes all it's arguments, puts them into an array, dispatches to a Func<object?[], object?> delegate to do the actual work, and unboxes the result - doing all of that without runtime codegen).
| <linker> | ||
| <assembly fullname="System.Linq.Expressions"> | ||
| <type fullname="System.Linq.Expressions.LambdaExpression"> | ||
| <method signature="System.Boolean get_CanCompileToIL()" feature="System.Linq.Expressions.CanCompileToIL" featurevalue="false" body="stub" value="false" /> |
There was a problem hiding this comment.
Why new feature switch for code compilation mode? Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).
There was a problem hiding this comment.
#17973 (comment) has reasoning for why this needs to be different from RuntimeFeature.IsDynamicCodeSupported.
Also the defaults don't look correct for existing configurations (e.g. published trimmed console app).
Could you clarify?
There was a problem hiding this comment.
has reasoning for why this needs to be different from
I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.
Could you clarify?
What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.
There was a problem hiding this comment.
I don't see the reasoning in the comment and this mode is not new in .NET7. In .NET6 we use the same options to detect NativeAOT like setup where IsDynamicCodeSupported indicates that code generation is possible with IsDynamicCodeCompiled specifying how it such generated code processed.
We could make this so that the body of CanCompileToIL is implemented as RuntimeFeature.IsDynamicCodeSupported, but I'm a believer in separating behavioral changes from refactoring changes and this would be a behavioral change. It can happen in subsequent pull request as a thing that can be discussed separately from this refactoring.
What does happen today when you publish trimmed console for desktops. I guess it has "CanCompileToIL" set to true and here we are setting it to false for everything.
The substitution is set as feature="X" featurevalue="false" body="stub" value="false" - if I'm reading IL Linker sources right, this substitution will be ignored unless someone sets feature X to false. Is that correct reading? I wasn't able to build intuition behind what featurevalue=X featuredefault=Y means and always defer to reading linker source code. If my interpretation is correct, this will do the right thing for trimmed apps (IL compiler will be kept).
src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/DelegateHelpers.cs
Outdated
Show resolved
Hide resolved
|
@MaximLipnin I can't make much sense from the runtime-manual run because it seems to have many unrelated failures. Could you please see if there's anything needing my attention? |
@MichalStrehovsky JFYI - By default, only |
|
@cston @jaredpar Could you take a look at this PR to change compile-time |
|
@vitek-karas could you double check the runtime feature flag stuff here? |
| @@ -15,7 +11,9 @@ public static class InterpreterTests | |||
| { | |||
| private static readonly PropertyInfo s_debugView = typeof(LightLambda).GetPropertyAssert("DebugView"); | |||
|
|
|||
| [Fact] | |||
| // FEATURE_COMPILE is not directly required, | |||
There was a problem hiding this comment.
Fixed those all.
Once this is merged, I'll do a test pass on CoreCLR with the IL Compiler disabled and see whether these comments are correct. If the comment is correct, I'll change the condition from "is IL compiler available" to "is Reflection.Emit available" and delete the comment, because that's what we're after.
| #if FEATURE_COMPILE // When we don't have FEATURE_COMPILE we don't have the Reflection.Emit used in the tests. | ||
|
|
||
| [Theory, ClassData(typeof(CompilationTypes))] | ||
| // When we don't have FEATURE_COMPILE we don't have the Reflection.Emit used in the tests. |
| @@ -268,9 +268,11 @@ public void ImplicitlyTyped() | |||
| double, double, double, double, | |||
| bool>), exp.Type); | |||
|
|
|||
| #if FEATURE_COMPILE | |||
| // From this point on, the tests require FEATURE_COMPILE (RefEmit) support as SLE needs to create delegate types on the fly. | |||
ghost
locked as resolved and limited conversation to collaborators
System.Linq.Expressions currently offers multiple build time definitions to build different flavors of the library:
FEATURE_COMPILE(defined everywhere except iOS/tvOS/Catalyst, and NativeAOT) - enables Reflection.Emit-based expression tree compiler.FEATURE_INTERPRET(always defined and not actually possible to build without) - enables the expression interpreterFEATURE_DLG_INVOKE(defined everywhere except NativeAOT, but we likely need to be able to run without it on iOS too because there's uninvestigated bugs around it ActiveIssue'd in Make LambdaCompiler prefer interpretation to compilation on iOS #54970) - in the interpreter, use a delegate to invokeCallInstructionsinstead ofMethodInfo.Invoke. The delegate might have to be Reflection.Emitted if it's not supportable withFunc/Action(that's the uninvestigated iOS/tvOS/Catalyst bug).For #61231, we need to be able to build a single System.Linq.Expression library that can switch between these build-time configurations at publish time since we don't want to build a separate S.L.Expressions library for NativeAOT. There are advantages in having this setup for non-NativeAOT scenarios too. This pull request accomplishes that by mechanically changing the
#defines into feature switches.The feature switch is placed in the last proposed location for #17973. I expect we'll want such API to be public at some point; now that Xamarin and NativeAOT use this formerly .NET Native-only thing the API request became relevant again.
This pull request is focused on the mechanical replacement of
#defineswith feature switches and it's already a lot bigger than I'm comfortable with.There's some obvious "
!FEATURE_COMPILEmeans this is .NET Native with everything what that meant" that I did not touch because this is meant to be a mechanical change. Some cleanup will be needed at some point. Right now this just mostly means we're running fewer tests than we could.Validation:
437436 kB, the diffs are expected).Cc @jkotas @marek-safar @MaximLipnin @steveisok @akoeplinger